Repository navigation
Fix filters on a table identity-partitioned on a uuid column - #4370
rahulsmahadev wants to merge 1 commit into
Conversation
Manifests are read with uuid partition values as uuid.UUID, while UUID literals hold the 16 bytes, so partition filters and residuals on a uuid identity partition never matched: EqualTo and In planned no files, and NotEqualTo and NotIn planned every file. Give partition values the literal representation before the partition and residual evaluators compare them. Co-authored-by: Isaac <no-reply@databricks.com>
Fokko
left a comment
Fokko
left a comment
There was a problem hiding this comment.
Thanks for digging into this! I think the root cause is that we have two in-memory representations of a uuid: the Avro UUIDReader returns uuid.UUID (and #4360 asserts that for partition[0]), while UUIDLiteral and from_bytes(UUIDType) use bytes. Rather than converting at the evaluator call sites, I'd prefer to standardize on uuid.UUID. This is also what Java does: UUIDLiteral is a Literal<UUID> and Conversions.fromByteBuffer returns a UUID.
Some pointers:
UUIDLiteral: wrap auuid.UUIDinstead ofbytes.ser_modelalready returns aUUID.literal()andStringLiteral.to(UUIDType): stop calling.bytes.FixedLiteral.to(UUIDType)andBinaryLiteral.to(UUIDType): buildUUID(bytes=...), and convert back with.bytesinUUIDLiteral.to(FixedType)/UUIDLiteral.to(BinaryType).from_bytes(UUIDType): returnuuid.UUID(bytes=b), so the metrics evaluators also compare the lower and upper bounds asUUID._to_partition_representation(UUIDType): keep identity values asUUIDinstead of converting them tobytes._convert_scalar: convert aUUIDto.byteswhen building thepa.uuid()scalar for the Arrow filter.
Ordering is preserved: UUID compares by its integer value, which matches unsigned big endian byte order, so range predicates and min/max pruning keep working. The visible change is that UUIDLiteral.value becomes a UUID, which is worth a note in the changelog.
This also covers the projected identity values, where Record(field_value) is built from the partition value and compared against the literal. That path has the same mismatch and isn't touched by this PR.
| return pa.RecordBatchReader.from_batches(target_schema, batches).cast(target_schema) | ||
|
|
||
|
|
||
| def _partition_with_literal_values(partition_type: StructType) -> Callable[[Record], Record]: |
There was a problem hiding this comment.
I don't think we should patch the Record here. It feels like we're working around the actual issue: partition values are uuid.UUID while UUIDLiteral holds bytes. Converting at two call sites in the planner means every other place that evaluates a partition Record still has to know about this. For example, the projected identity values in visitors.py build Record(field_value) from the partition value and run into the same mismatch. It also allocates a new Record for every data file on uuid specs, and adds another spec keyed cache to plan_files. If we make both representations agree (see my comment above), this helper can go away.
Rationale for this change
Scan planning gets uuid identity partitions wrong. On a table partitioned by
identity(u)on auuidcolumn:Manifests are read with uuid partition values as
uuid.UUID, while aUUIDLiteralholds the 16 bytes, so the partition evaluator and the residual evaluator compare aUUIDwithbytesand never find them equal.EqualToandInplan no files, andNotEqualToandNotInplan every file. Engines that use PyIceberg's planning (plan_files()) get the same wrong set of files, andcount()is wrong because it trusts the residual.This change gives partition values the representation literals use (a uuid becomes its 16 bytes) before the partition and residual evaluators compare them. It only applies to specs with a uuid partition field; other partition values are passed through unchanged.
Follow-up to #4360, which noted this as a separate fix. Reading the matching files still hits the PyArrow
ArrowNotImplementedErrorof #2372:to_arrow()raises it, and so doescount()when it has to read the files (a row predicate that the partition does not resolve, or delete files). For=andinfilters this replaces silently returning no rows.Are these changes tested?
Yes.
test_scan_plans_uuid_identity_partitionsintests/io/test_pyarrow.py, next to the append test from #4360, checks the planned partitions andcount()for=,in,!=andnot infilters on a uuid identity partition. It fails on main (the=filter plans no files) and passes with this change.Are there any user-facing changes?
Bug fix: the partition and residual evaluators compare uuid partition values with uuid literals correctly. No API changes.
This pull request and its description were written by Isaac.